Reviewed 71 of 71 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 508 at r1 (raw file):

                Spacer(modifier = Modifier.height(Dimens.cellLabelVerticalPadding))
                HeaderSwitchComposeCell(
                    title = "DAITA",

Should be an untranslatable string resource I think?

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 510 at r1 (raw file):

                    title = "DAITA",
                    isToggled = state.isDaitaEnabled,
                    isEnabled = true,

Is true by default so not needed

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/SelectLocationViewModel.kt line 143 at r1 (raw file):

                    add(FilterChip.Provider(providerCountFilter))
                }
                if (settings?.tunnelOptions?.wireguard?.daita == true) {

Maybe a bit nice with some kind of extension function like Settings.hasDaita(): Boolean ?

Reviewed 51 of 71 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @albin-mullvad)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 508 at r1 (raw file):

                Spacer(modifier = Modifier.height(Dimens.cellLabelVerticalPadding))
                HeaderSwitchComposeCell(
                    title = "DAITA",

Should be resource?

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayItemExtensions.kt line 73 at r1 (raw file):

}

fun RelayItem.Location.Country.applyFilters(

Should we call it just filter? With applyFilters it sounds like we apply a filter to a Country, thus that it would be added on top of the country.

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/DaitaSettings.kt line 3 at r1 (raw file):

package net.mullvad.mullvadvpn.lib.model

enum class DaitaSettings {

If we only have On/Off, we should make this a Boolean.

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/util/AssetToFilesDirExtractor.kt line 7 at r1 (raw file):

import java.io.FileOutputStream

class AssetToFilesDirExtractor(val context: Context) {

Do we even need to have this instance? We should be able to just make it a function right? Maybe it can also be and extension function on Context if just plan to use it within the scope of a Context.

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/util/AssetToFilesDirExtractor.kt line 8 at r1 (raw file):

class AssetToFilesDirExtractor(val context: Context) {
    fun extract(assetName: String, overwriteFileIfAssetMoreRecent: Boolean = false) {

Do we ever user anything else than true? Is the flag needed?

Reviewable status: 58 of 74 files reviewed, 7 unresolved discussions (waiting on @Pururun and @Rawa)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 508 at r1 (raw file):

Fixed! 👍

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 508 at r1 (raw file):

Fixed! 👍

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 510 at r1 (raw file):

This was copy pasta from other toggles were we also seem to explicitly set it, but removed it from this one now

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayItemExtensions.kt line 73 at r1 (raw file):

Done

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/SelectLocationViewModel.kt line 143 at r1 (raw file):

Done.

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/DaitaSettings.kt line 3 at r1 (raw file):

Done.

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/util/AssetToFilesDirExtractor.kt line 7 at r1 (raw file):

Done.

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/util/AssetToFilesDirExtractor.kt line 8 at r1 (raw file):

Done.

Reviewed 36 of 36 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @albin-mullvad and @Rawa)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 182 at r2 (raw file):

    }

    daitaConfirmationDialogResult.OnNavResultValue { doEnableDaita ->

I wonder if we should move this code to the dialog to not increase the complexity of VpnSettings

Reviewed 1 of 36 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DaitaConfirmationDialog.kt line 48 at r2 (raw file):

                modifier = Modifier.fillMaxWidth().height(Dimens.dialogIconHeight),
                painter = painterResource(id = R.drawable.icon_alert),
                contentDescription = "",

contentDescription = null, seems more reasonable than ""

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 521 at r2 (raw file):

                    title = stringResource(id = R.string.daita),
                    isToggled = state.isDaitaEnabled,
                    onCellClicked = { newValueIsEnable ->

Name is pretty weird newValudIsEnable, I think it is fine that it is just enable, it is implicit that it is the new value.

Reviewable status: 73 of 75 files reviewed, 3 unresolved discussions (waiting on @Pururun and @Rawa)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DaitaConfirmationDialog.kt line 48 at r2 (raw file):

Done! Copy pasta artifact, so we might want to clean that up in other places as well

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 182 at r2 (raw file):

Went with the other approach at first, but felt like way too much boiler plate for such a trivial call where the result is always either to cancel or just call a single vm function (always with true as an argument). Me and David discussed it and agreed to go with this approach for now, but we said that we might want to eventually take a step back and restructure some parts of the vpn settings screen. Does that sound OK?

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 521 at r2 (raw file):

Done.

Reviewed 3 of 3 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/FilterChip.kt line 67 at r10 (raw file):

        border =
            FilterChipDefaults.filterChipBorder(
                borderColor = borderColor,

Should probably set disabledBorderColor as well since it is transparent.

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/FilterChip.kt line 75 at r10 (raw file):

        label = { Text(text = text, style = MaterialTheme.typography.labelMedium) },
        trailingIcon = {
            if (enabled) {

Trailing icon takes nullable composable, so emitting a empty composable count result in some unexpected padding to be added. If we don't have a icon we should not emit any lambda at all.

Reviewable status: 75 of 78 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98, @Pururun, and @Rawa)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/FilterChip.kt line 67 at r10 (raw file):

Done.

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/FilterChip.kt line 75 at r10 (raw file):

Done.

Reviewed 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved